-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Translations update #1545
Translations update #1545
Conversation
…into translations-update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @emlys , just a few quick questions and comments.
workbench/readme.md
Outdated
|
||
6. Replace `src/main/i18n/$LL.[json,po]` and `src/renderer/i18n/$LL.json` with the updated versions received from the translator | ||
``` | ||
|
||
7. Commit the changes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like step 6 was removed and that this should be the new step 6.
@@ -2,3 +2,4 @@ Sphinx>=1.3.1,!=1.7.1 | |||
sphinx-rtd-theme | |||
sphinx-intl | |||
sphinx-reredirects | |||
pyyaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What relies on the pyyaml
dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -196,11 +228,11 @@ class DataDownloadModal extends React.Component { | |||
)} | |||
/> | |||
<Form.Check.Label> | |||
{modelName} | |||
{displayNames[modelName]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch to include translations for these. I'm thinking about having to remember to update this list manually if model names change or are added / removed. Since we're using modelName
here, is it possible to somehow programmatically wrap modelName
with the t
designation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`${t(modelName)}`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, t(...)
should always go around a string literal because it's used to extract strings for translation. If it goes around a variable, there's no way to know what strings need to be translated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @emlys , looks like there's just a conflict in HISTORY to fix and it should be good!
Description
Fixes #1623
Checklist